Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix useSubscription bug in React v18 StrictMode (#9664) #9707

Merged

Conversation

kazekyo
Copy link
Contributor

@kazekyo kazekyo commented May 11, 2022

Fix #9664

React18 automatically unmounts and remounts components in StrictMode. When React remount a component, the previous state is restored. However, if the previous state is reused, the subscription will not work.
So we should recreate the necessary object when React remounts the component.

For more information on why we should not reuse the object, see #9664 (comment)


Currently, Apollo Client includes React17 in its devDependencies.

I have made this fix and confirmed that it works in a sample application.
At this time, I changed Apollo Client's React version to 18.
But I confirmed that some unrelated tests failed due to the impact of updating React to 18, so I did not commit package.json.

Therefore, I have not included in this commit the additional tests needed to prove the fix in React18.

@apollo-cla
Copy link

@kazekyo: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@benjamn benjamn added this to the v3.6.x patch releases milestone May 11, 2022
@benjamn benjamn requested review from brainkim and benjamn May 11, 2022 14:20
@kazekyo
Copy link
Contributor Author

kazekyo commented May 11, 2022

I don't know how to reduce the file size. Please let me know if there is anything I need to do 🙏

@benjamn
Copy link
Member

benjamn commented May 12, 2022

Also, no need to worry about the ci/circleci; Filesize check when it's a close call like this. We'll adjust it later.

@benjamn
Copy link
Member

benjamn commented May 12, 2022

One more request, if you have time: if you can add a regression test (in this module) that fails without these changes, that will help us make sure we don't regress this behavior (without realizing it) in the future.

@kazekyo
Copy link
Contributor Author

kazekyo commented May 13, 2022

@benjamn

if you can add a regression test (in this module) that fails without these changes, that will help us make sure we don't regress this behavior (without realizing it) in the future.

I agree. I think the following test should be added:
kazekyo@8569139#diff-7bca58c8d152cf9947df8bd35ec7ae4286339c99d95afb9437447128f7724791R529-R531

But I have two problems 😅

First, I upgraded React version to 18, but in this test, the component is not remounted even if StrictMode is enabled. So the test is always PASS. I do not know what reason this is occurring.
Do you know of a way around this problem?

Second, this test requires React v18. I do not know of any way to reproduce this situation other than using React v18 StrictMode.
I confirmed that some unrelated tests failed due to the effect of upgrading React to 18.

Test Suites: 10 failed, 106 passed, 116 total
Tests:       20 failed, 15 skipped, 1740 passed, 1775 total
Snapshots:   135 passed, 135 total
Time:        33.893 s, estimated 39 s
Ran all test suites.

Should we fix all the failed tests with this PR?

@benjamn
Copy link
Member

benjamn commented May 16, 2022

It sounds like we need to start a React 18-only test suite, to accompany our existing React ≤17-compatible test suite. I can work on that today!

React18 automatically unmounts and remounts components in StrictMode. When React remount a component, the previous state is restored. However, if the previous state is reused, the subscription will not work.
So we should recreate the necessary object when React remounts the component.
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will find a way to properly test this code in multiple major versions of React, but that does not need to block shipping these changes. Thanks for your thoughtful analysis and for coming up with this solution @kazekyo!

@benjamn benjamn force-pushed the fix_usesubscription_in_strict_mode branch from c504248 to e62f8f6 Compare May 16, 2022 21:02
@benjamn benjamn merged commit 4571e1a into apollographql:main May 16, 2022
benjamn added a commit that referenced this pull request May 16, 2022
brainkim pushed a commit that referenced this pull request May 23, 2022
@jurgelenas
Copy link

We will find a way to properly test this code in multiple major versions of React, but that does not need to block shipping these changes. Thanks for your thoughtful analysis and for coming up with this solution @kazekyo!

I think apollo-client has an issue when re-mounting subscription components without the StrictMode now.

This issue is reproducible on our open source desktop app ExpressLRS Configurator. We were able to fix subscription re-mount issues in development build by wrapping whole app inside <React.StrictMode> component. See ExpressLRS/ExpressLRS-Configurator#351. But nightly builds are still not working at https://github.com/ExpressLRS/ExpressLRS-Configurator-Nightlies.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useSubscription stop working in React v18 StrictMode
5 participants